mcv/pkg/preflightcheck/vllm.go (2)

119-124: Use logging instead of fmt.Printf for consistency.

Same issue as above - use logging.Debugf for consistency with the rest of the codebase.

♻️ Suggested fix

 		if backendMatches && warpMatches {
 			matched = true
 			// For detailed arch compatibility, rely on Summary label check
-			fmt.Printf("Binary cache entry matches GPU: backend=%s, warpSize=%d\n",
-				backend, expectedWarpSize)
+			logging.Debugf("Binary cache entry matches GPU: backend=%s, warpSize=%d",
+				backend, expectedWarpSize)
 			break
 		}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@mcv/pkg/preflightcheck/vllm.go` around lines 119 - 124, The code uses
fmt.Printf in the GPU-matching branch (inside the block checking backendMatches
and warpMatches where matched is set) which breaks logging consistency; replace
the fmt.Printf call that prints "Binary cache entry matches GPU: backend=%s,
warpSize=%d\n" with logging.Debugf using the same formatted message and
variables (backend, expectedWarpSize), keeping the matched=true and break
behavior unchanged and ensuring any fmt import is removed if no longer used.

---

76-80: Use logging instead of fmt.Printf for consistency.

The rest of the codebase uses the logging package. Using fmt.Printf here is inconsistent and won't respect log level settings.

♻️ Suggested fix

 	// Log the AOT cache entries for debugging
 	for _, entry := range entries {
-		fmt.Printf("AOT compile cache: hash=%s, rank=%s, size=%d bytes\n",
-			entry.Hash, entry.Rank, entry.FileSize)
+		logging.Debugf("AOT compile cache: hash=%s, rank=%s, size=%d bytes",
+			entry.Hash, entry.Rank, entry.FileSize)
 	}

Note: You'll need to add the logging import.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@mcv/pkg/preflightcheck/vllm.go` around lines 76 - 80, Replace the fmt.Printf
usage in the AOT cache logging loop with the project's logging package: locate
the loop that iterates over entries (for _, entry := range entries) and change
the print to use logging.<level> (e.g., logging.Debug or logging.Infof depending
on desired verbosity) and include entry.Hash, entry.Rank and entry.FileSize in
the log message; also add the logging import to the file. Ensure the message
format and log level are consistent with other uses of logging in this package.

mcv/pkg/cache/vllm.go (2)

520-523: Move GPU detection outside the inner loop to avoid redundant calls.

detectActualGPUInfo() is called once per VLLMCacheMetadata entry, but it performs expensive operations (config initialization, device registry access, device startup). Since GPU info doesn't change between iterations, move the call outside both loops.

♻️ Suggested fix

 func buildBinaryCacheSummary(metadata []VLLMCacheMetadata) (*Summary, error) {
 	targetMap := make(map[string]SummaryTargetInfo)
+
+	// Detect actual GPU from the system once (not per entry)
+	// NOTE: We detect the actual system GPU rather than trusting VLLM_TARGET_DEVICE
+	// because caches may be copied from other systems
+	detectedBackend, detectedArch, detectedWarpSize, detectedPTX := detectActualGPUInfo()

 	for _, meta := range metadata {
 		if meta.CacheFormat != BinaryCacheFormat {
 			continue
 		}

-		// Detect actual GPU from the system once per metadata entry
-		// NOTE: We detect the actual system GPU rather than trusting VLLM_TARGET_DEVICE
-		// because caches may be copied from other systems
-		detectedBackend, detectedArch, detectedWarpSize, detectedPTX := detectActualGPUInfo()
-
 		for i := range meta.BinaryCacheEntries {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@mcv/pkg/cache/vllm.go` around lines 520 - 523, The code currently calls
detectActualGPUInfo() inside the loop that iterates VLLMCacheMetadata, causing
expensive duplicate GPU detection; move the call so it runs once before entering
the outer/inner loops (call detectActualGPUInfo() once and store
detectedBackend, detectedArch, detectedWarpSize, detectedPTX in local variables)
and then use those stored values inside the loop(s) instead of calling
detectActualGPUInfo() repeatedly; update any references in the loop to use the
precomputed variables and remove the inner detectActualGPUInfo() invocation.

---

350-350: Move regex compilation outside the loop.

The rankDirRegex is recompiled on every iteration of the outer loop. Move it to a package-level variable or outside the loop.

♻️ Suggested fix

+var aotRankDirRegex = regexp.MustCompile(`^rank_\d+_\d+$`)
+
 func detectAOTCompileCache(aotPath string) ([]AOTCompileCacheMetadata, error) {
 	var aotCaches []AOTCompileCacheMetadata
 	// ...
 	for _, hashEntry := range entries {
 		// ...
-		rankDirRegex := regexp.MustCompile(`^rank_\d+_\d+$`)
 		for _, rankEntry := range rankEntries {
 			// ...
-			if !rankDirRegex.MatchString(rankEntry.Name()) {
+			if !aotRankDirRegex.MatchString(rankEntry.Name()) {
 				continue
 			}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@mcv/pkg/cache/vllm.go` at line 350, The regex rankDirRegex is being compiled
inside a loop causing repeated allocations; move its compilation out of the loop
(e.g., declare a package-level var or compute once before the loop) and reuse
that compiled *regexp. Ensure you replace the inline
regexp.MustCompile(`^rank_\d+_\d+$`) inside the loop with the shared variable
(referencing rankDirRegex) used by the functions/methods in vllm.go that iterate
directories.

mcv/docs/vllm-binary-cache.md (1)

747-747: Add language specifier to fenced code block.

The static analysis flagged this code block as missing a language specifier.

♻️ Suggested fix

-```
+```text
 ┌─────────────────────────┐

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@mcv/docs/vllm-binary-cache.md` at line 747, The fenced code block containing
the ASCII art is missing a language specifier; update the opening fence from ```
to ```text (or another appropriate language) for the ASCII art block in
vllm-binary-cache.md so static analysis recognizes it — locate the ASCII-art
fenced block (the triple-backtick surrounding "┌─────────────────────────┐") and
change the opening fence to include "text".

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

mcv/pkg/preflightcheck/vllm.go (2)

76-80: Use logging framework instead of fmt.Printf for consistency.

The codebase uses logging.Infof/logging.Debugf (logrus) for output. Using fmt.Printf bypasses log levels and formatting.

♻️ Proposed fix

 	// Log the AOT cache entries for debugging
 	for _, entry := range entries {
-		fmt.Printf("AOT compile cache: hash=%s, rank=%s, size=%d bytes\n",
+		logging.Debugf("AOT compile cache: hash=%s, rank=%s, size=%d bytes",
 			entry.Hash, entry.Rank, entry.FileSize)
 	}

This requires adding the logging import if not already present.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@mcv/pkg/preflightcheck/vllm.go` around lines 76 - 80, Replace the fmt.Printf
calls in the AOT cache logging loop with the project's logging package (use
logging.Infof or logging.Debugf as appropriate) so log levels and formatting are
consistent; update the loop that iterates over entries (the for _, entry :=
range entries block) to call logging.Debugf("AOT compile cache: hash=%s,
rank=%s, size=%d bytes", entry.Hash, entry.Rank, entry.FileSize) and add the
logging import if it's not already present.

---

119-124: Use logging framework here as well.

Same issue as above—use logging.Debugf for consistency.

♻️ Proposed fix

 		if backendMatches && warpMatches {
 			matched = true
 			// For detailed arch compatibility, rely on Summary label check
-			fmt.Printf("Binary cache entry matches GPU: backend=%s, warpSize=%d\n",
+			logging.Debugf("Binary cache entry matches GPU: backend=%s, warpSize=%d",
 				backend, expectedWarpSize)
 			break
 		}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@mcv/pkg/preflightcheck/vllm.go` around lines 119 - 124, Replace the
fmt.Printf call in the vllm preflight loop (where backendMatches and warpMatches
are checked and matched is set) with the logging framework: call logging.Debugf
to emit the same message and values (backend, expectedWarpSize), and add or
ensure the logging package is imported; update the print site in the code block
that currently prints "Binary cache entry matches GPU: backend=%s, warpSize=%d"
so it uses logging.Debugf for consistency with other debug output.

mcv/docs/vllm-binary-cache.md (1)

747-747: Add language specifier to fenced code block.

Static analysis flags this block as missing a language specifier. Since it's an ASCII diagram, use text or plaintext.

♻️ Proposed fix

-```
+```text
 ┌─────────────────────────┐
 │  1. Start vLLM          │

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@mcv/docs/vllm-binary-cache.md` at line 747, The fenced code block containing
the ASCII diagram (the block starting with triple backticks and lines like
"┌─────────────────────────┐" and "│  1. Start vLLM          │") is missing a
language specifier; update the opening fence from ``` to ```text (or
```plaintext) so the block becomes a text/plaintext fenced code block to satisfy
static analysis.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

mcv/pkg/cache/vllm.go (1)

540-542: ⚠️ Potential issue | 🟡 Minor

Fix fallback path to update arch when GPU detection fails.

Lines 552-554 update the backend when detection fails, but leave arch as the string "UnknownBackend" from failed detection. This causes malformed arch strings like "sm_UnknownBackend" (line 541) when the fallback backend resolves to CUDA.

The arch should either be:

1. Reset to a sensible default (e.g., "75" for CUDA), or
2. Extracted from cache metadata alongside the backend

Example fix: update arch in fallback path

 			// Handle special cases where no GPU is detected
 			if backend == UnknownBackend {
 				logging.Warn("Could not detect GPU on system, using cache metadata as fallback")
 				// Fallback to cache metadata if GPU detection failed
 				backend = binaryCache.TargetDevice
 				if backend == "" {
 					backend = CUDABackend // Default if not specified
 				}
+				// Reset arch to default since detection failed
+				arch = "75" // Safe default; consider extracting from cache if available
 				// Set default warp sizes

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@mcv/pkg/cache/vllm.go` around lines 540 - 542, The fallback path that resets
backend leaves arch set to the failed-detection sentinel (producing values like
"sm_UnknownBackend"); update the fallback logic so that when backend is
reassigned to CUDABackend you also update arch to a sensible CUDA default (e.g.,
"75") or load the arch from the cache metadata at the same time; modify the code
handling variables backend and arch (the block that currently checks backend ==
CUDABackend and formats arch with "sm_%s") so arch is corrected when the
fallback backend is applied.

mcv/pkg/cache/vllm.go (1)

524-527: GPU detection called redundantly inside loop.

detectActualGPUInfo() is called inside the metadata loop (line 527), meaning it's invoked once per VLLMCacheMetadata entry. Since the system's GPU doesn't change during execution, this detection should be performed once before the loop.

♻️ Suggested refactor to detect GPU once

 func buildBinaryCacheSummary(metadata []VLLMCacheMetadata) (*Summary, error) {
 	targetMap := make(map[string]SummaryTargetInfo)

+	// Detect actual GPU from the system once
+	// NOTE: We detect the actual system GPU rather than trusting VLLM_TARGET_DEVICE
+	// because caches may be copied from other systems
+	detectedBackend, detectedArch, detectedWarpSize, detectedPTX := detectActualGPUInfo()
+
 	for _, meta := range metadata {
 		if meta.CacheFormat != BinaryCacheFormat {
 			continue
 		}

-		// Detect actual GPU from the system once per metadata entry
-		// NOTE: We detect the actual system GPU rather than trusting VLLM_TARGET_DEVICE
-		// because caches may be copied from other systems
-		detectedBackend, detectedArch, detectedWarpSize, detectedPTX := detectActualGPUInfo()
-
 		for i := range meta.BinaryCacheEntries {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@mcv/pkg/cache/vllm.go` around lines 524 - 527, detectActualGPUInfo() is being
called inside the per-entry metadata loop (where VLLMCacheMetadata entries are
processed), causing redundant system GPU detection; move the call out of the
loop so detectedBackend, detectedArch, detectedWarpSize, detectedPTX are
computed once before iterating over the metadata slice/loop and reuse those
values for each VLLMCacheMetadata entry. Update any references inside the loop
to use the precomputed variables and remove the in-loop call to
detectActualGPUInfo().

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

mcv/docs/vllm-binary-cache.md (2)

14-20: ⚠️ Potential issue | 🟠 Major

Make the AOT compile support status and cache format consistent.

The doc says MCV supports VLLM_USE_AOT_COMPILE=1 at Lines 14-20, but later says it is unsupported at Lines 35-46, 107-112, and 511-513. The format table also maps AOT to "binary", while the implementation now emits/validates cacheFormat: "aot_compile". Please update these sections to consistently describe VLLM_USE_AOT_COMPILE=1 as supported with the explicit "aot_compile" manifest format, and keep “Mega AOT” separate from AOT compile.

📝 Proposed wording direction

-**Note**: The `VLLM_USE_AOT_COMPILE=1` workflow uses a different structure at
-`torch_compile_cache/torch_aot_compile/{hash}/rank_{rank}_{dp_rank}/model` and is
-not currently supported by MCV.
+**Note**: The `VLLM_USE_AOT_COMPILE=1` workflow uses a different structure at
+`torch_compile_cache/torch_aot_compile/{hash}/rank_{rank}_{dp_rank}/model`.
+MCV supports this layout as `cacheFormat: "aot_compile"`; compatibility is
+validated primarily through the image summary label because the AOT metadata
+contains limited hardware details.

-| AOT | Binary files | `"binary"` | `"binary"` | `"binary"` |
+| AOT compile | Single `model` file | `"aot_compile"` | N/A | `"binary"` |

Also applies to: 35-46, 107-112, 477-483, 511-513

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@mcv/docs/vllm-binary-cache.md` around lines 14 - 20, Update the documentation
so AOT compile (VLLM_USE_AOT_COMPILE=1) is consistently described as supported:
change the "AOT Compile Support" paragraph to state it is supported, replace any
claims that it is unsupported with support wording, and ensure the
manifest/format table and any references use cacheFormat: "aot_compile" instead
of "binary"; keep the separate concept/name "Mega AOT" distinct from AOT compile
in all sections (including the format mapping and compatibility notes) and
ensure the preflight/cache validation text references the summary label and
limited hardware metadata for the "aot_compile" format.

---

332-348: ⚠️ Potential issue | 🟡 Minor

Update CUDA summary examples to use the sm_ architecture format.

The docs show summary labels with "arch": "75", but buildBinaryCacheSummary prefixes CUDA architecture as sm_, so the emitted summary is expected to look like "arch": "sm_75". Please align the examples and the explanatory text with the actual label format.

📝 Proposed documentation fix

 {
   "backend": "cuda",
-  "arch": "75",
+  "arch": "sm_75",
   "warp_size": 32,
   "ptx_version": 590,
   "cuda_version": "12.9"
 }

-**Important**: Notice that the `arch` field shows the **actual GPU compute capability** (e.g., `75` for Tesla T4 which is sm_7.5), not the CUDA toolkit version.
+**Important**: Notice that the `arch` field shows the **actual GPU compute capability** in CUDA `sm_` format (e.g., `sm_75` for Tesla T4 / sm_7.5), not the CUDA toolkit version.

Also applies to: 642-651

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@mcv/docs/vllm-binary-cache.md` around lines 332 - 348, Update the CUDA
architecture examples and explanatory text to use the "sm_" prefix that
buildBinaryCacheSummary produces (e.g., "arch": "sm_75" instead of "75"); change
the JSON example and all descriptive mentions (including the sentence about
compute capability and any other examples around the same section) to the sm_
format, and ensure references to environment variables remain correct
(VLLM_TARGET_DEVICE for backend, VLLM_MAIN_CUDA_VERSION for CUDA version) so
labels emitted by buildBinaryCacheSummary match the documentation.

mcv/pkg/cache/vllm.go (1)

636-655: ⚠️ Potential issue | 🟠 Major

Populate arch when falling back to cache metadata.

When GPU detection fails, backend and warpSize are replaced from cache metadata/defaults, but arch remains UnknownBackend. That produces a summary target like backend=cuda, arch=UnknownBackend, which will fail architecture compatibility checks later. Add an environment-derived architecture fallback or omit the target when architecture cannot be determined.

🐛 Proposed fix direction

 			if backend == UnknownBackend {
 				logging.Warn("Could not detect GPU on system, using cache metadata as fallback")
 				// Fallback to cache metadata if GPU detection failed
 				backend = binaryCache.TargetDevice
 				if backend == "" {
 					backend = CUDABackend // Default if not specified
 				}
+				if envArch, ok := binaryCache.Env["VLLM_PAGED_ATTN_ARCH"].(string); ok && envArch != "" {
+					arch = envArch
+				}
+				if backend == CUDABackend && arch != "" && !strings.HasPrefix(arch, "sm_") {
+					arch = fmt.Sprintf("sm_%s", arch)
+				}
 				// Set default warp sizes

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@mcv/pkg/cache/vllm.go` around lines 636 - 655, When falling back because
backend == UnknownBackend, the code updates backend and warpSize but leaves arch
as UnknownBackend which breaks later compatibility checks; update the fallback
block in vllm.go (the section handling UnknownBackend) to also populate arch
from cache or environment (e.g., use binaryCache.TargetArch or an env var) or
clear/omit arch when it cannot be determined, and ensure any defaulting logic
(mapping backend->arch) is applied so the resulting target does not contain
arch==UnknownBackend; change references around backend, arch, warpSize,
UnknownBackend, binaryCache.TargetDevice and CUDABackend accordingly.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes